HHH-19750, HHH-19753 Fix criteria value-bind parameter and SqmFunction equals and hashing logic #10874
Conversation
|
Awesome, thanks for working on this, @mbellade! |
| @Override | ||
| // TODO: fix this | ||
| public int compareTo(SqmParameter<T> parameter) { | ||
| return this == parameter ? 0 : 1; | ||
| return Integer.compare( hashCode(), parameter.hashCode() ); | ||
| } | ||
|
|
||
| // this is not really a parameter, it's really a literal value | ||
| // so use value equality based on its value | ||
|
|
||
| @Override | ||
| public boolean equals(Object object) { | ||
| return object instanceof ValueBindJpaCriteriaParameter<?> that | ||
| && Objects.equals( this.value, that.value ); | ||
| // && getJavaTypeDescriptor().areEqual( this.value, (T) that.value ); | ||
| return this == object; | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return value == null ? 0 : value.hashCode(); // getJavaTypeDescriptor().extractHashCode( value ); | ||
| return super.hashCode(); |
There was a problem hiding this comment.
This approach is strictly-speaking correct, but I believe it results in an unacceptable number of cache misses.
Please see the discussion in https://hibernate.atlassian.net/browse/HHH-19556.
There was a problem hiding this comment.
Sure, the same discussion was brought up again in https://hibernate.atlassian.net/browse/HHH-19753 - but it's a matter of correctness: the value-bind parameters are bound by instance-identity, and their type is inferred based on their use-sites, so considering two parameters with the same value the same was not correct (other than leading to the error reported in https://hibernate.atlassian.net/browse/HHH-19750).
There was a problem hiding this comment.
As I said to you on Zulip, I'm pretty sure that if everything else in the query is identical except for, say, the type of a VBJCP, then they really are the same query. (For the purposes of query interpretation caching.)
That is to say, I don't see how you could have two different queries which are identical in every other respect except for the VBJCP.
I think the issue actually arises from how they're used as map keys in JdbcParameterBindingsImpl, and maybe other places you pointed out in Zulip.
So really the problem is that we're trying to use the same equals()/hashCode() for comparing SQM tree equality as we use for comparing parameter equality in JdbcParameterBindingsImpl. And it seems to me that maybe the easiest fix to that is to use something else to compare parameters in JdbcParameterBindingsImpl.
But that's no more than a guess/suggestion. I have not tried it, of course.
| public boolean equals(Object other) { | ||
| return other instanceof SqmFunction<?> that | ||
| && Objects.equals( this.functionName, that.functionName ) | ||
| if ( this == other ) { | ||
| return true; | ||
| } | ||
| if ( other == null || getClass() != other.getClass() ) { | ||
| return false; | ||
| } | ||
|
|
||
| final SqmFunction<?> that = (SqmFunction<?>) other; | ||
| return Objects.equals( this.functionName, that.functionName ) | ||
| && Objects.equals( this.arguments, that.arguments ); |
There was a problem hiding this comment.
What's the reason for this change?
IMO, if the function name and the arguments are the same, it's the exact same function invocation.
The only case where that's not the case is with "fancy" aggregate functions (window functions and ordered set aggregates), and they are, I believe, all subclasses of SelfRenderingSqmFunction which overrides this implementation of equals().
So I believe this implementation is already correct.
There was a problem hiding this comment.
Do you mean the class check? There are other SqmFunction subtypes, other than window and ordered set aggregate, that might have the same name / arguments but be of different type - I know semantically it wouldn't make sense to have the same function name for differently typed functions, but the instance type check is very easy to perform and generally a good practice.
There was a problem hiding this comment.
There are other SqmFunction subtypes, other than window and ordered set aggregate, that might have the same name / arguments but be of different type
Yes but that shouldn't matter. If the name and arguments of the function are the same, it's the same function.
What I'm saying is: I don't think we ever need to look at expression types when comparing two SQM trees for equality. Equality is sufficiently determined by structure, and by identifier names. Types are inferred from that information.
There was a problem hiding this comment.
Perhaps mine was an excess of caution, in principle your implementation should work fine. To be clear, I was explaining the reasoning behind my change, but I'm ok with switching back to the old implementation, I won't push back on this.
There was a problem hiding this comment.
It's really not a big deal. I'm much more concerned about the ValueBindJpaCriteriaParameter issue.
4 participants
https://hibernate.atlassian.net/browse/HHH-19750
https://hibernate.atlassian.net/browse/HHH-19753
While working on the fix I noticed some tests failures due to incorrect query-cache hits when using value-bind criteria parameters for values and/or literals, which led me to HHH-19753 which I tried fixing as well in the last commit.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.